Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ABI checks: add support for loongarch #133249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heiher
Copy link
Contributor

@heiher heiher commented Nov 20, 2024

LoongArch psABI1 specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack.

This patch addresses the following warnings:

warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI
    --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5
     |
3695 |     __lsx_vreplgr2vr_b(a)
     |     ^^^^^^^^^^^^^^^^^^^^^ function called here
     |
     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
     = note: for more information, see issue #116558 <https://github.com/rust-lang/rust/issues/116558>
     = note: `#[warn(abi_unsupported_vector_types)]` on by default

r? @workingjubilee

Footnotes

  1. https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 20, 2024
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

lsx? lasx? oh dear.

@heiher
Copy link
Contributor Author

heiher commented Nov 20, 2024

lsx? lasx? oh dear.

LSX is a 128-bit vector extension, and LASX is a 256-bit vector extension. Parameters and return values for neither of these data types are passed through vector registers. 😃

@workingjubilee
Copy link
Member

Then why is the lint triggering?

@workingjubilee
Copy link
Member

@workingjubilee
Copy link
Member

This is not the correct fix, the correct fix is to fix the lint itself.

@heiher
Copy link
Contributor Author

heiher commented Nov 20, 2024

Here is the uses_vector_registers check.

It looks fairly dubious to me. https://github.com/rust-lang/rust/blame/bfe809d93c67de03e3a84b80549927fd828ec5d0/compiler/rustc_monomorphize/src/mono_checks/abi_check.rs#L43

I see it. But I'm not sure how to make it more accurate.

Here's a case:

#![feature(stdarch_loongarch)]
use std::arch::loongarch64::*;

pub unsafe fn simd(s: i32) -> u32 {
    lsx_vpickve2gr_bu::<0>(lsx_vreplgr2vr_b(s))
}

and the PassMode is

Direct(ArgAttributes { regular: , arg_ext: None, pointee_size: Size(0 bytes), pointee_align: None })

BackendRepr is

Vector { element: Initialized { value: Int(I8, true), valid_range: 0..=255 }, count: 16 }

@heiher
Copy link
Contributor Author

heiher commented Nov 21, 2024

Is it true that the Rust frontend cannot determine whether a parameter is passed through a vector register or a general-purpose register based solely on PassMode and BackendRepr? @veluca93 This decision is made by the LLVM target backend. If so, adding ABI checks to LoongArch is also a valid solution, and at least harmless.

@workingjubilee
Copy link
Member

My understanding is the frontend technically cannot precisely control it, no.

LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose
registers, while LASX vector types are passed indirectly through the stack.

This patch addresses the following warnings:

```
warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI
    --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5
     |
3695 |     __lsx_vreplgr2vr_b(a)
     |     ^^^^^^^^^^^^^^^^^^^^^ function called here
     |
     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
     = note: for more information, see issue rust-lang#116558 <rust-lang#116558>
     = note: `#[warn(abi_unsupported_vector_types)]` on by default
```

[^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
@heiher
Copy link
Contributor Author

heiher commented Nov 25, 2024

Since we cannot determine the type of registers used for vectors in Rust's front-end, I have reverted the return type of features_for_correct_vector_abi to Option. A return value of None indicates that the given target does not use vector registers for vectors.

Do you think this is ok? @workingjubilee

@workingjubilee
Copy link
Member

Hmm.

@workingjubilee
Copy link
Member

What we want is to both

  • understand what the C ABI uses
  • understand what is possible at the level of codegen and the machine ISA, because we also need to do codegen correctly for intrinsics and other, non-C ABIs

If the backend does not expose some ability to handle register usage (which does imply that we should have a nonzero ability to get it wrong), then I think we will simply fuck things up in a different way from our current approaches.

@workingjubilee
Copy link
Member

workingjubilee commented Nov 25, 2024

In either version of this PR that I have seen so far, it adjusts things in a way that fixes things for the C ABI in certain conditions. However, the motivation1 for the changes are that our current compiler contract with the Loongarch backend remains poorly-specified and poorly-understood.

  • The first version proposed, accordingly, seemed to just assuming that our compiler contract is similar to the contract with the other backends, when it apparently is not?
  • The second version proposes surrendering to the whims of the codegen backend, in a way which may be technically correct but leaves us in the dark as we head into the future.

So, before I merge any fixes, I would like to understand, relative to how the Loongarch backend handles the LLVMIR lowering,

  • what we can do
  • what the effects of those choices are
  • what we should be doing

Footnotes

  1. I mean, the motivation strictly speaking is that we would like to not lint unnecessarily, but it is revealing that we do not understand exactly what is necessary.

Amanieu added a commit to Amanieu/stdarch that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants